-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/disallow dynamic property option #1234
Feature/disallow dynamic property option #1234
Conversation
I'll add some tests for the feature. |
It might be better to make this as an option for |
I worry that this logic needs to be deep in NodeScopeResolver, or not? If not then yeah, parameter for a rule is nicer. |
1ab1af1
to
e64bc9a
Compare
I think it's possible now. |
While this option can optionally restore the behavior before #1223 (comment), it would restore the false positive phpstan/phpstan#3171 too. |
Should be fixed by specifying types in isset. Related issue phpstan/phpstan#3171
Thank you! Can you please send an update to https://phpstan.org/config-reference#stricter-analysis as well? There's an "Edit this page" link at the bottom :) I'll update phpstan-strict-rules so that the playground with strict rules + bleedingEdge enabled will have this option on. |
I'm testing this in the real world and it's really nicely done! The impact is really minimal and exactly in the right weird places where I'd expect it 👍 |
Thanks! I'm glad that I could help solving dynamic properties related issues 😄 |
Implemented bool option in
NodeScopeResolver
as mentioned in #1223 (comment) to optionally restore the behavior before #1223.Strictly, it is not exactly the same behavior as before because it disallows dynamic properties in
isset
andempty
too, but I believe making them consistent is better.Also, fixes phpstan/phpstan#3171 because it's related.